Dev115 #201
Dev115 #201
Conversation
|
Hi John, do I understand correctly that the ability to have XUL in the options page for an addon is now gone? No more |
|
Yes, as part of the conversion step, I removed the WindowListener API and moved the options page from XUL to HTML. The XUL options page was a feature of the WindowListener API, to quickly reduce the fallout of the removed "legacy" overlay support in TB78 and was intended as a short rescue mechanism. XUL is being removed, so add-on developers, who did not do any further updates after the initial WindowListener API update, constantly have to adapt to the changing/reduced XUL support. To move this project forward and to help identify the real needed WebExtension APIs, I did the HTML conversion now and dropped the WindowListener API. The XUL tree is indeed one of the biggest native XUL feature, which does not have a direct HTML counterpart. But it is doable of course, either by researching and finding a 3rd party HTML/JS library, which does that, or by implementing a custom solution. The questions is: do we need it? Would it not be sufficient to directly modify the real folder tree by shortcuts (move up/down) and context menu entries (move up/down, sort alpha, sort invert alpha, sort normal)? |
|
Was this correction fixed on the Supernova? |
There was a problem hiding this comment.
I reviewed the code and it looks good, just added some nitpicking comments.
Unfortunately it does not work for me: I have several account but only for one I sorted the folders. Now when updating to TB 115 and this extension:
- Only the one account which I sorted folders on is shown. By chance this is the first account ("account1")
- No folders are shown for this account
- In the "manually sort folders" config page only this account is shown and again no folders.
In the debugger I can find this error:
Uncaught (in promise) TypeError: preferences.accountSettings.has is not a function
load moz-extension://deac0eff-b0ff-40e0-a470-372ea19d121a/options/options.js:299
async* moz-extension://deac0eff-b0ff-40e0-a470-372ea19d121a/options/options.js:331
Please let my know how I can support you in debugging/fixing this.
| let arr = new TextEncoder().encode(data); | ||
| let str = ""; | ||
| for (let i = 0; i < arr.length; i += 65536) { | ||
| str += String.fromCharCode.apply(null, arr.subarray(i, i + 65536)); | ||
| } | ||
| return btoa(str); |
There was a problem hiding this comment.
Due to the return in line 5 these lines are unused and can be removed.
This is also worth a warning for TB:
unreachable code after return statement options.js:7:4
|
|
||
|
|
||
| /** | ||
| * M-C has still not enabled "svg.context-properties.content.enabled" by default. |
There was a problem hiding this comment.
What is "M-C"? Please spell out.
|
|
||
| if (!preferences.accountSettings.has(accountId)) { | ||
| preferences.accountSettings.set(accountId, { | ||
| type: "0", |
There was a problem hiding this comment.
Please add a brief comment what a type of zero means. thx.
| } | ||
| } | ||
| // Move the actual DOM elements, update sort state. | ||
| function on_folder_move_down(e) { |
There was a problem hiding this comment.
As a plus on_folder_move_up/down could be merged into a single function called e.g. swap_folders(a, b).
|
|
||
| if (!preferences.accountSettings.has(account.id)) { | ||
| preferences.accountSettings.set(account.id, { | ||
| type: "0", |
There was a problem hiding this comment.
Please add a brief comment what a type of zero means. thx.
| } | ||
|
|
||
| async function install(window, preferences) { | ||
| for (let i = 0; i < 20; i++) { |
There was a problem hiding this comment.
Please add a brief comment like "wait up to 2500 seconds to folder pane to appear"
| } | ||
| break; | ||
|
|
||
| case "1": |
There was a problem hiding this comment.
Please add a brief comment which case this is, as done above. Same for the remaining cases.
| } | ||
|
|
||
| async function uninstall(window) { | ||
| for (let i = 0; i < 20; i++) { |
There was a problem hiding this comment.
Please add a timeout comment here, to.
|
When removing the |
See discussion in #199.
Convert add-on to be more WebExtension-ish by getting rid of the WindowListener API and convert locales to WebExtension locales and options page to HTML.
I am not yet happy with how I handle the SVGs. As stated in comments, WebExtensions cannot use core SVGs because core uses a feature which is not yet standardized and disabled for WebExtensions. The solution tried in this PR is aiming at including the unchanged SVG from core. But performance is bad, so I propose to include "normal" SVG and modify the core ones.
Alternatively, the entire sort UI in the options page could be removed and replaced by context menu entries in the folder pane: move up/down, sort alpha, sort invert alpha, sort normal. This would also solve the issue, that the current PR does not allow to sort subfolder entries (only top level entries) and the UI would need significat updates to support that. Drag'n'Drop for sorting needs an Experiment, which I try to avoid at this stage.
This PR does not attempt to decode the sort order stored in the pref branch, but starts fresh and stores sort order in local storage.